Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Export maven.repo.local for gradle tests #17643

Merged
merged 1 commit into from
Jun 3, 2021
Merged

Export maven.repo.local for gradle tests #17643

merged 1 commit into from
Jun 3, 2021

Conversation

glefloch
Copy link
Member

@glefloch glefloch commented Jun 2, 2021

This generate a gradle.properties file in user.home/.gradle.
This file contains a system property which will then be used by gradle.

It should fix gradle tests in the release job.

@quarkus-bot quarkus-bot bot added area/cli Related to quarkus cli (not maven/gradle/etc.) area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins labels Jun 2, 2021
@glefloch glefloch requested review from ebullient and gsmet June 2, 2021 20:47
@ebullient
Copy link
Member

It seems really strange that we have to reach through in this way..

I wonder if we're dropping environment variables across the gradle invoke.
Should we try propagating the ENV vars instead (MAVEN_LOCAL_REPO, MAVEN_OPTS, GRADLE_OPTS)?

quarkus/devtools/cli/src/main/java/io/quarkus/cli/build/ExecuteUtil.java

Line 69 in cc94f11

Process process = new ProcessBuilder()

@glefloch
Copy link
Member Author

glefloch commented Jun 3, 2021

@ebullient I looked at the gradle code, environment variables are not used in the local repository path resolution:

https://github.com/gradle/gradle/blob/2a87114a9ea1876541673f37b42293c2ac914548/subprojects/dependency-management/src/main/java/org/gradle/api/internal/artifacts/mvnsettings/DefaultLocalMavenRepositoryLocator.java#L44

gsmet
gsmet requested changes Jun 3, 2021
View reviewed changes
devtools/cli/src/test/java/io/quarkus/cli/CliProjectGradleTest.java Outdated
Properties gradleProperties = new Properties();
gradleProperties.put("systemProp.maven.repo.local", localMavenRepo);
try (FileOutputStream os = new FileOutputStream(propertyFile)) {
gradleProperties.store(os, "Gradle property file");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that you change the global Gradle configuration of the user. That won't fly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, only, if the maven.repo.local system property is set. I can backup the user file and rollback to it after the test?

Copy link
Member

@famod famod Jun 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That won't fly.

+1! I have proxy settings in that file and I certainly don't expect Quarkus to modify it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it was definetely too aggressive. I reverted that part as it was not necessary.

@famod
Copy link
Member

famod commented Jun 3, 2021

Is there really no other way to pass the repo? E.g. via -D... arg?

@glefloch
Copy link
Member Author

glefloch commented Jun 3, 2021

right, there is already a special case for tests. I will try to set something there.

@glefloch glefloch force-pushed the fix/release-test branch from 88f63a9 to cf4032e Compare June 3, 2021 10:17
@glefloch
Copy link
Member Author

glefloch commented Jun 3, 2021

Actually I misread the error in the github report. Gradle tests are OK with the -Dmaven.repo.local @ebullient added, it's the start/stop of the daemon that fail.
I added a system property to avoid having the error in the log. I still wonder if we need to explicitly start/stop the daemon.

@quarkus-bot
Copy link

quarkus-bot bot commented Jun 3, 2021 •
edited
Loading

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building cf4032e

Status Name Step Test failures Logs Raw logs
✖ JVM Tests - JDK 11 Build Test failures Logs Raw logs
✖ JVM Tests - JDK 11 Windows Build Test failures Logs Raw logs
✔️ JVM Tests - JDK 16

Full information is available in the Build summary check run.

Test Failures

⚙️ JVM Tests - JDK 11 #

📦 devtools/cli

✖ io.quarkus.cli.CliProjectGradleTest.testExtensionList line 74 - More details - Source on GitHub

✖ io.quarkus.cli.CliProjectGradleTest.testCreateAppDefaults line 74 - More details - Source on GitHub

✖ io.quarkus.cli.CliProjectGradleTest.testCreateAppOverrides line 74 - More details - Source on GitHub

⚙️ JVM Tests - JDK 11 Windows #

📦 devtools/cli

✖ io.quarkus.cli.CliProjectGradleTest.testExtensionList line 74 - More details - Source on GitHub

✖ io.quarkus.cli.CliProjectGradleTest.testCreateAppDefaults line 74 - More details - Source on GitHub

✖ io.quarkus.cli.CliProjectGradleTest.testCreateAppOverrides line 74 - More details - Source on GitHub

@glefloch glefloch force-pushed the fix/release-test branch from cf4032e to 33bab86 Compare June 3, 2021 12:11
@gsmet gsmet merged commit e3cd280 into quarkusio:main Jun 3, 2021
@quarkus-bot quarkus-bot bot added this to the 2.1 - main milestone Jun 3, 2021
@gsmet
Copy link
Member

gsmet commented Jun 3, 2021

Thanks!

@glefloch glefloch deleted the fix/release-test branch June 3, 2021 13:48
@gsmet gsmet modified the milestones: 2.1 - main, 2.0.0.Final Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@famod famod famod left review comments

@ebullient ebullient ebullient approved these changes

@gsmet gsmet gsmet approved these changes

Assignees
No one assigned
Labels
area/cli Related to quarkus cli (not maven/gradle/etc.) area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins
Projects
None yet
Milestone
2.0.0.Final
Development

Successfully merging this pull request may close these issues.
4 participants
@glefloch @ebullient @famod @gsmet